The test is running but it fails only because of the pyaml config file which is not up to date with the current pyaml version.

Hello @JeanLucPons, @TeresiaOlsson, @gubaidulinvadim, and @patrickmadela. The first version of the integration tests is ready with a simple lattice (thanks @GamelinAl).
The tests are very basic for now, but they will allow us to move forward on this basis.

Hi,
Thanks for this simple FODO with sextu.
It would be nice to configure a tune and chroma tuning tool.

Hi, Thanks for this simple FODO with sextu. It would be nice to configure a tune and chroma tuning tool.

Yes, but for now, I just want to validate this integration test implementation. Additional tests will be added in a future PR, and we can discuss the necessary tests then.

Ok
Are the UUID strictly necessary ?
The writing will not be very convenient if i want to get a handle to a magnet.

Ok Are the UUID strictly necessary ? The writing will not be very convenient if i want to get a handle to a magnet.

No, I would like to update the lattice to use better names. Once again, the purpose here is to set up the integration test mechanism. The data and tests can be updated later.

IMHO, It is preferable to start with good dataset if possible.
I let you do as you want.

Is the idea of the integration test that every time someone pushes to the repository it will run the test once for the TANGO version and once for the EPICS version? And then later also maybe for the DOOCS version?

I'm wondering if there is some way we can come up with a mock-up naming convention for the control system names that allows us to generate the yaml files for each control system as part of the test? Then we just need to maintain the lattice and that naming convention and not yaml files for each specific control system.

For EPICS the naming convention is very forgiving so maybe we can just generate EPICS PV names based on the TANGO attribute names?

Is the idea of the integration test that every time someone pushes to the repository it will run the test once for the TANGO version and once for the EPICS version? And then later also maybe for the DOOCS version?

Yes, for now it's just TANGO, I need to work on the simulator to use EPICS properly.

I'm wondering if there is some way we can come up with a mock-up naming convention for the control system names that allows us to generate the yaml files for each control system as part of the test? Then we just need to maintain the lattice and that naming convention and not yaml files for each specific control system.

It need to be evaluated. Actually, the files for integration teses are generated from the lattice.

For EPICS the naming convention is very forgiving so maybe we can just generate EPICS PV names based on the TANGO attribute names?

We should look at it, yes.

Sounds like a good idea then that we try it first for TANGO and when that is working we try to generate the EPICS version based on the same configuration.

At least for me the EPICS version is not urgent.

@JeanLucPons, @GamelinAl and I are looking for a more complete lattice. For example, the way to use FamName and UUID is not trivial. I'll try to come up with a better proposal tomorrow. In the meantime, I would like you to refocus the review on the test architecture.

My main problem is that to run the integration test locally i have to change each time the 2 config files which is not convenient at all. This is why i propose a solution to avoid any prefix (tango host) definition in tests.
A second problem is that it is not trivial to extract the tango host from the twin due to logging.

OK for ophyd asynch i had to uninstall re install. It is difficult for me to run the twin. Why not in this PR? Why not start with good architecture ?

I think @gupichon is trying to follow your principle (a good one) of doing small PRs :) The architecture of the test is to be improved (in this PR). It is true that running the twin locally is not very optimal now (to be fixed).

Extensive logging will be fixed too. We already gave this feedback to Waheed (and there are fewer logs now than there were before, but still too many).

My main problem is that to run the integration test locally i have to change each time the 2 config files which is not convenient at all.

It's corrected.

I think @gupichon is trying to follow your principle (a good one) of doing small PRs :) The architecture of the test is to be improved (in this PR). It is true that running the twin locally is not very optimal now (to be fixed).

What I propose is to reduce the PR by avoiding config duplication and allow dynamic tango host change.

Extensive logging will be fixed too. We already gave this feedback to Waheed (and there are fewer logs now than there were before, but still too many).

Good

A second problem is that it is not trivial to extract the tango host from the twin due to logging.

As I said, I will write a great script that mimics what the pipeline does, featuring clean logging messages with the full TANGO host and a 'ready to use' notification.

It's corrected.

If i understand well your patch, it will then rely on TANGO_HOST env variable which is not good for us and that will fail with EPICS.

My main problem is that to run the integration test locally i have to change each time the 2 config files which is not convenient at all. This is why i propose a solution to avoid any prefix (tango host) definition in tests. A second problem is that it is not trivial to extract the tango host from the twin due to logging.

I agree. I have experienced the same problems with the EPICS twin. I will check with @Sulimankhail what he thinks about the option to dynamically generate the tango host similarly to how it's done for the EPICS twin. But if that feature is added it might affect what the host becomes when using it in a Github workflow. Maybe for the workflow we still want the option to have a fixed port?

As it is a CI, the solution should work straightforward for all use case (github runner) and local test.
Otherwise, it will be a nightmare.

OK for ophyd asynch i had to uninstall re install. It is difficult for me to run the twin. Why not in this PR? Why not start with good architecture ?

I think @gupichon is trying to follow your principle (a good one) of doing small PRs :) The architecture of the test is to be improved (in this PR). It is true that running the twin locally is not very optimal now (to be fixed).

Extensive logging will be fixed too. We already gave this feedback to Waheed (and there are fewer logs now than there were before, but still too many).

Noted

It's corrected.

If i understand well your patch, it will then rely on TANGO_HOST env variable which is not good for us and that will fail with EPICS.

How do you want to do it then?
For EPICS, I'm still working on the simulator side, but I lack time, to be honest.

How do you want to do it then? For EPICS, I'm still working on the simulator side, but I lack time, to be honest.

Using fragment (ConfigurationManager) with dynamic config for the CS.
It will also allow to avoid the duplication of fodo_1gev_6d_pyaml-oa.yaml and fodo_1gev_6d_pyaml.yaml.

It's corrected.

If i understand well your patch, it will then rely on TANGO_HOST env variable which is not good for us and that will fail with EPICS.

How do you want to do it then? For EPICS, I'm still working on the simulator side, but I lack time, to be honest.

I think the idea is that the twin will generate it's own port based on the environment it is started in. Like how for the EPICS twin it reads the username from the system and puts that as the prefix for all the PVs.

And then we need to add in the pyAML configuration loader that it understands how to resolve environment variables. So you for example for the TANGO_HOST field can write 127.0.0.1:$UID and it will resolve what the environment variable is.

I think the idea is that the twin will generate it's own port based on the environment it is started in. Like how for the EPICS twin it reads the username from the system and puts that as the prefix for all the PVs.

That is actually how the twin works. I'm just forcing it for the integration test.

And then we need to add in the pyAML configuration loader that it understands how to resolve environment variables. So you for example for the TANGO_HOST field can write 127.0.0.1:$UID and it will resolve what the environment variable is.

The TANGO host and EPICS prefix are handled by the backends, so I'm not quite sure I follow what you are proposing here.

Using fragment (ConfigurationManager) with dynamic config for the CS.
It will also allow to avoid the duplication of fodo_1gev_6d_pyaml-oa.yaml and fodo_1gev_6d_pyaml.yaml.

I'll do that in the next PR with the new tests. In the meantime, setting the environment variable in the pipeline is a good approach anyway for the scope of this PR.

I think the idea is that the twin will generate it's own port based on the environment it is started in. Like how for the EPICS twin it reads the username from the system and puts that as the prefix for all the PVs.

That is actually how the twin works. I'm just forcing it for the integration test.

And then we need to add in the pyAML configuration loader that it understands how to resolve environment variables. So you for example for the TANGO_HOST field can write 127.0.0.1:$UID and it will resolve what the environment variable is.

The TANGO host and EPICS prefix are handled by the backends, so I'm not quite sure I follow what you are proposing here.

So when you start the TANGO twin locally it will already have a user-specific port?

My idea was just that when you give the host and prefix in the yaml file part of the string can be an environment variable which will be resolved when loading in the configuration. For the BESSY II examples I for example always need to manually switch pons to my username on my computer before I can run the example. Everyone at the workshop had to do the same and they complained about it. Instead in the yaml file you can write $USER (or some other syntax to indicate that it's an environment variable) and it will be resolved during runtime.

So when you start the TANGO twin locally it will already have a user-specific port?

You can set it as a parameter. If you don't, it will use the first available port on localhost.

My idea was just that when you give the host and prefix in the yaml file part of the string can be an environment variable which will be resolved when loading in the configuration. For the BESSY II examples I for example always need to manually switch pons to my username on my computer before I can run the example. Everyone at the workshop had to do the same and they complained about it. Instead in the yaml file you can write $USER (or some other syntax to indicate that it's an environment variable) and it will be resolved during runtime.

I understand now, but this needs to be done in the pyaml-cs-oa project. Or maybe in pyaml, if we want to introduce a specific grammar detection like we do for wildcards or filenames. Either way, it needs to be addressed in a separate issue.

My idea was just that when you give the host and prefix in the yaml file part of the string can be an environment variable which will be resolved when loading in the configuration. For the BESSY II examples I for example always need to manually switch pons to my username on my computer before I can run the example. Everyone at the workshop had to do the same and they complained about it. Instead in the yaml file you can write $USER (or some other syntax to indicate that it's an environment variable) and it will be resolved during runtime.

By the way, it's a very good idea.

My idea was just that when you give the host and prefix in the yaml file part of the string can be an environment variable which will be resolved when loading in the configuration. For the BESSY II examples I for example always need to manually switch pons to my username on my computer before I can run the example. Everyone at the workshop had to do the same and they complained about it. Instead in the yaml file you can write $USER (or some other syntax to indicate that it's an environment variable) and it will be resolved during runtime.

By the way, it's a very good idea.

Actually, it's not really my idea... I think it was part of the feedback from the workshop. It needs to be added to pyaml but no one had time to do it yet. I can try to do it after I have finished the PR for the schema registry.

Using fragment (ConfigurationManager) with dynamic config for the CS.
It will also allow to avoid the duplication of fodo_1gev_6d_pyaml-oa.yaml and fodo_1gev_6d_pyaml.yaml.

I'll do that in the next PR with the new tests. In the meantime, setting the environment variable in the pipeline is a good approach anyway for the scope of this PR.

Why you don't wan't to reduce this PR ?
And there's no need to modify either pyaml-cs-ca or tango-pyaml to have dynamic prefix or tango-host.

Why you don't wan't to reduce this PR ?

I already answered this point twice, but you might have missed it.
You can find it here:
#295 (comment)
And here:
#295 (comment)
I don't want to spend any more time on this since it will be removed anyway.

And there's no need to modify either pyaml-cs-ca or tango-pyaml to have dynamic prefix or tango-host.

How do you want to do it then? With the ConfigurationManager? I think what @TeresiaOlsson wants is to extract the value from the environment.

I don't want to spend any more time on this since it will be removed anyway.

OK do as you want. May be you prefer to have duplicate code because it is easier.
I spent much time in code reviews and trying to factor config files and you come with new duplicates.
That's mean that for the next PR the review will be harder, removal of duplication plus new tests plus i don't know what.
I remove myself from the reviewers of this PR.

@JeanLucPons, I made the script so the twin is easier to launch locally.

I think we merge it in so we can start using the workflow infrastructure. There are changes needed to be able to run the test locally but those are better to address in a separate PR.

I created a script used both in the pipeline and to run the twin locally.